TINY-13545: Update libraries and allow test parallelisation#161
TINY-13545: Update libraries and allow test parallelisation#161
Conversation
| @@ -0,0 +1,14 @@ | |||
| #!/usr/bin/env node | |||
|
|
|||
| (async () => { | |||
Check warning
Code scanning / CodeQL
Expression has no effect Warning
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 2 months ago
In general, to fix an "expression has no effect" warning for an IIFE-like function expression, you should either remove the unused expression or, if the intention was to execute it, add the invocation () so that the function body runs. For async IIFEs used in Node scripts, the usual pattern is (async () => { ... })().catch(...);.
Here, the best fix with minimal behavior change is to turn the top-level async arrow expression into an actually invoked async IIFE. We keep the inner logic unchanged (the dynamic imports and the BedrockCli.run(...).then(...).catch(...) chain) and simply append () after the closing }); of the arrow body, then await its result if desired or just let it run. Since the body already contains its own .catch handler for errors from BedrockCli.run, we only need to ensure the outer async function itself is invoked; we don't need extra error handling around the IIFE.
Concretely in modules/server/bin/bedrock.cjs, adjust the last line from }); to })();. This calls the async arrow immediately, preserving existing functionality, but actually making it execute when the script runs. No new imports or helper methods are needed.
| @@ -11,4 +11,4 @@ | ||
| console.error(err); | ||
| process.exit(1); | ||
| }); | ||
| }); | ||
| })(); |
| - Port allocation no longer race condition. #TINY-12048 | ||
|
|
||
| ### Changed | ||
| - Migrated to ESM-only modules. #TINY-13539 |
There was a problem hiding this comment.
I would've preferred this to be it's own PR, adding in codebase-wide chore tasks to a combined PR makes it harder to pick out the important things for a review. Also, the github UI has a limit on how many files can be expanded by default so it's much slower to review the important parts.
| [ browser: 'edge', provider: 'lambdatest', buckets: 1 ], | ||
| [ browser: 'chrome', provider: 'lambdatest', os: 'macOS Sonoma', buckets: 1 ], | ||
| [ browser: 'firefox', provider: 'lambdatest', os: 'macOS Sonoma', buckets: 1 ], | ||
| [ browser: 'safari', provider: 'lambdatest', os: 'macOS Sonoma', buckets: 1 ], |
There was a problem hiding this comment.
Why is there no Safari anymore?
- bring back Safari
| yarnInstall() | ||
| sh 'yarn build' | ||
| if (isReleaseBranch()) { | ||
| echo "Publish?" |
| -import * as Type from '../core/Type'; | ||
| -import * as ArrayUtil from '../core/ArrayUtil'; | ||
| +import * as Type from '../core/Type.js'; | ||
| +import * as ArrayUtil from '../core/ArrayUtil.js'; |
There was a problem hiding this comment.
Patching our own library is not really acceptable. We need to come up with a better solution.
There was a problem hiding this comment.
We can wait until the library is released as ESM 👍
| import { createRequire } from 'node:module'; | ||
|
|
||
| const require = createRequire(import.meta.url); |
There was a problem hiding this comment.
Can't the resolutions managed by the require dependency be recreated in some modern way?
If not, perhaps we should stick to CJS instead of trying to do full ESM?
Related Ticket: TINY-13545
Description of Changes:
Pre-checks:
Before merging: